Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[PileupJetId, Puppi] Backport of #40762 (Pileup ID input variable fix, puppi weight ValueMap access, optional photon protection for existing puppi weights) to CMSSW_13_0_X #40801

Merged
merged 9 commits into from
Feb 21, 2023
Merged

[PileupJetId, Puppi] Backport of #40762 (Pileup ID input variable fix, puppi weight ValueMap access, optional photon protection for existing puppi weights) to CMSSW_13_0_X #40801

merged 9 commits into from
Feb 21, 2023

Conversation

jshin96
Copy link
Contributor

@jshin96 jshin96 commented Feb 17, 2023

Backport from #40762

Original PR description:

This PR has three main modifications:

Pileup Jet Id variable bug fix.
PileupJetIdProducer and PileupJetIdAlgo are used to compute input variables and the final discriminant for
the Pileup jet Id BDT. There was a bug for one of the variable calculation such that when there is no charged
constituent inside a jet, it assigns the very last constituent in the list of constituent as leading charged
constituent. This PR corrects this error and assigns zero pt and large phi and eta when there is no charged
constituent.

Correct puppi weight access implementation by ValueMap.
Previously, the puppi weight for each constituent was accessed from packedCandidate object in the code, but for the
compatibility with other codes in CMSSW, this weight must be implemented through ValueMap. In this PR, puppi
weight retrieval from ValueMap is implemented, just like PR #40667. As in that PR, the naming of "puppi weight" is
generalized to "constituent weight". Furthermore, there are parts of PileupJetIdAlgo where comparison of constituent's pt
(in order to find, for example, leading charged constituent) was done incorrectly when weighted pt was compared with
unweighted pt. This error is also fixed.

The ValueMap implementation does not affect CHS jets. However, the fix in leading charged constituent selection could
have affected CHS jets. In further detail, CHS jets have more constituents in each jet in general and it is very rare
for CHS jets to have no charged constituent at all, so the effect of such fix was insignificant.

Add an option in PuppiProducer to apply photon protection for existing weights.
Previously the photon protection is applied also on existing puppi weights. This will confuse users who would want
PuppiProducer to provide ValueMap of weights with the same exact weights stored in packedPFCandidates in MiniAODs.
This PR adds a flag (default set to False) to enable photon protection on existing puppi weights.

PR validation:

Validation plots comparing the input variables for Pileup Jet Id training before and after fix can be found in this JMAR meeting contribution.
Passes the usual runTheMatrix test: runTheMatrix.py -l limited -i all --ibeos. Test done by @nurfikri89.
Passes reMiniAOD and reNanoAOD workflows: runTheMatrix.py -i all --ibeos -l 1325.518,2500.312. Test done by @nurfikri89.
Passes the JMENano workflows: runTheMatrix.py -i all --ibeos -l 10224.15,11024.15,25202.15,11634.15. Test done by @nurfikri89.

@cmsbuild cmsbuild added this to the CMSSW_13_0_X milestone Feb 17, 2023
@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 17, 2023 •
edited
Loading

A new Pull Request was created by @jshin96 for CMSSW_13_0_X.

It involves the following packages:

  • CommonTools/PileupAlgos (reconstruction)
  • PhysicsTools/NanoAOD (xpog)
  • RecoJets/JetProducers (reconstruction)

@cmsbuild, @mandrenguyen, @clacaputo, @swertz, @vlimant can you please review it and eventually sign? Thanks.
@rappoccio, @jdolen, @missirol, @yslai, @AnnikaStein, @schoef, @jdamgov, @ahinzmann, @nhanvtran, @gkasieczka, @clelange, @hatakeyamak, @gpetruc, @mariadalfonso, @seemasharmafnal this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@jshin96 jshin96 changed the title From1300pre4 pujetid fix [PileupJetId, Puppi] Backprot of #40762 (Pileup ID input variable fix, puppi weight ValueMap access, optional photon protection for existing puppi weights) to CMSSW_13_0_X Feb 17, 2023
@jshin96 jshin96 changed the title [PileupJetId, Puppi] Backprot of #40762 (Pileup ID input variable fix, puppi weight ValueMap access, optional photon protection for existing puppi weights) to CMSSW_13_0_X [PileupJetId, Puppi] Backport of #40762 (Pileup ID input variable fix, puppi weight ValueMap access, optional photon protection for existing puppi weights) to CMSSW_13_0_X Feb 17, 2023
@swertz
Copy link
Contributor

swertz commented Feb 17, 2023

enable nano

@swertz
Copy link
Contributor

swertz commented Feb 17, 2023

please test

@nurfikri89
Copy link
Contributor

Seems like the test is not triggered @swertz

@swertz
Copy link
Contributor

swertz commented Feb 17, 2023

please test

Just to be sure, but it's weird indeed, the tests-started label was added...

@perrotta
Copy link
Contributor

backport of #40762

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8552b8/30723/summary.html
COMMIT: 283d569
CMSSW: CMSSW_13_0_X_2023-02-17-1100/el8_amd64_gcc11
Additional Tests: NANO
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/40801/30723/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially removed 36 lines from the logs
  • Reco comparison results: 32 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3556272
  • DQMHistoTests: Total failures: 34
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3556216
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 213 log files, 164 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

NANO Comparison Summary

Summary:

  • You potentially removed 7 lines from the logs
  • Reco comparison results: 12 differences found in the comparisons
  • DQMHistoTests: Total files compared: 11
  • DQMHistoTests: Total histograms compared: 10829
  • DQMHistoTests: Total failures: 39
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 10790
  • DQMHistoTests: Total skipped: 0
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 10 files compared)
  • Checked 23 log files, 10 edm output root files, 11 DQM output files

Nano size comparison Summary:

Sample kb/ev ref kb/ev diff kb/ev ev/s/thd ref ev/s/thd diff rate mem/thd ref mem/thd
2500.31 2.233 2.232 0.000 ( +0.0% ) 9.66 9.57 +0.9% 1.480 1.490
2500.311 2.323 2.323 0.000 ( +0.0% ) 9.36 9.26 +1.1% 1.842 1.861
2500.312 2.277 2.277 -0.000 ( -0.0% ) 9.46 9.43 +0.3% 1.833 1.847
2500.33 1.099 1.100 -0.000 ( -0.0% ) 21.62 22.45 -3.7% 1.665 1.672
2500.331 1.394 1.394 0.000 ( +0.0% ) 15.97 16.34 -2.3% 1.821 1.816
2500.332 1.326 1.326 0.000 ( +0.0% ) 18.17 18.11 +0.4% 1.878 1.875
2500.401 2.139 2.139 0.000 ( +0.0% ) 10.52 10.41 +1.1% 1.165 1.187
2500.501 1.711 1.711 0.000 ( +0.0% ) 16.77 16.54 +1.4% 1.086 1.100
2500.511 1.124 1.124 0.000 ( +0.0% ) 30.35 30.97 -2.0% 1.336 1.351
2500.601 2.050 2.050 0.000 ( +0.0% ) 12.51 11.62 +7.7% 1.145 1.170

@swertz
Copy link
Contributor

swertz commented Feb 20, 2023

+1

Same changes (expected) as in #40762, needed for NanoV12 in 13_0.

@clacaputo
Copy link
Contributor

+reconstruction

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_13_0_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_13_1_X is complete. This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

@perrotta
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 1be1f46 into cms-sw:CMSSW_13_0_X Feb 21, 2023
@perrotta
Copy link
Contributor

I merged this too quickly by mistake (I wrongly imagined that the release in master was already merged and succesfully tested in CMSSW_13_1_X_2023-02-21-1100
Ok, I don't expect issues in fact, therefore there will quite likely be no need to revert it: just keep an eye on the CMSSW_13_1_X_2023-02-21-2300 IB

@cmsbuild cmsbuild mentioned this pull request Feb 22, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
backport-ok fully-signed orp-approved reconstruction-approved tests-approved xpog-approved
Projects
None yet
Milestone
CMSSW_13_0_X
Development

Successfully merging this pull request may close these issues.
6 participants
@jshin96 @cmsbuild @swertz @nurfikri89 @perrotta @clacaputo